MBS-7646: Show localized artist aliases in more places #3191
Conversation
|
@mwiencek @reosarevok @yvanzo Early feedback appreciated. :-) |
There was a problem hiding this comment.
I'm uploading this to get some feedback about whether this seems like a reasonable approach to pursue.
Thank you for looking into this issue!
Along with the TODOs in the code,
I didn’t review those.
I suspect that this code doesn't yet work for indexed searches
I confirm, but Solr search returns with artist results with aliases, so it is a pure MBS issue to make it work. See for example search API results with Pyotr Ilyich Tchaikovsky.
I've been testing it with direct searches in my local dev environment.
Works for me too: http://localhost:5000/search?query=Tchaikovsky&type=artist&method=direct
I'm also not sure if aliases should be displayed in more (or fewer) places, and whether there's any way to generalize this to support additional entity types instead of only artists.
@Aerozol would be the right person for providing some guidance, especially now that he can test PRs locally! 🚀
| if (comment) { | ||
| textParts.push(comment); | ||
| } | ||
| const text = textParts.join(', '); |
There was a problem hiding this comment.
For a localized comma-separated list, import our commaOnlyList and use it as follows:
| const text = textParts.join(', '); | |
| const text = commaOnlyList(textParts); |
There was a problem hiding this comment.
Thanks! I suspect that commaOnlyList can't be used now that I'm italicizing the alias (and the comma after it), but let me know if I'm wrong about that. Maybe I could do some hack like calling commaOnlyList([alias, ' ']) and then putting that inside the <i>.
|
I would love to test and feedback, but I think that would involve configuring the search server on my side? It was a bit of a rigmarole to get the test server running on Win at all so I'm a bit hesitant. I would love to see a screenshot - this sounds really useful! I am keen to see how it's displayed. |
|
@Aerozol no need for a search server; the localized names are shown in the results when you do a direct search (e.g. http://localhost:5000/search?query=tchaikovsky&type=artist&limit=25&method=direct). There also isn't too much to see, though; the disambiguation is just rendered as "Pyotr Ilyich Tchaikovsky, Russian romantic composer" instead of "Russian romantic composer". :-) |
|
Sorry for the delay in comment, I finally (with further help from monkey) managed to wade through all my Win OS/Ubuntu VM issues, and 'test' this. Hopefully nobody was waiting for me! This is a easy 'yes' from me. MB isn't mobile friendly anyway, so I don't see a downside in having this take up some of the empty horizontal space. And it is indeed a constant cause of friction re. disambiguation edits. My only thought is that we may want to distinguish between the automatically displayed content by putting it in italics, in case users get confused, but it would also be uglier. The TODO comments are too technical for me to address. My only wish is that this was extended to more (all) entity types/searches. And some day we maybe automate even more of the disambiguation, like RYM does - e.g. 'rock group from Bulgaria' can all be automated. cough bumps into derat cough Great stuff! |
|
Thanks for taking a look! I'll try to spend some more time soon working on the remaining issues, e.g. getting this to work for indexed search. It'd definitely be nice to support other entity types too, but I'll need to look into how hard it'll be to pass the aliases through there. And your suggestion to italicize the alias also seems like it'll help reduce confusion -- I'll try to see how that looks. One other consideration is that there are probably a bunch of entities that already have Latin names in their disambiguations where this would result in near-duplicated text. IIRC I was thinking of avoiding displaying the alias if it's already present in the disambiguation or if it matches the entity name, but there are likely still many cases that would be missed due to alternate spellings, diacritics, etc. |
|
Ninja'd! I was just drafting another comment that we might run into some situations where there is some very repetitive text. Because of text in the disambiguation, but also where the primary alias is very similar to the artist name (I don't know if this avoids adding it if it's 100% the same?) |
|
Avoiding adding it if the same as artist name makes a lot of sense to me. I would not hide it if it's already in the disambiguation - it should not be there anyway, so this will make it more obvious so people can remove it :) |
|
After some musicbrainz-docker help from @yvanzo in MBVM-95, I've started looking at this again. Here's a screenshot of what it looks like after italicizing the localized aliases:
|
|
I like the italics :) Another step towards clarity could be mouse-over text for the italics, something like: "This artists' primary alias in your [browser/account(?)] locale", but I don't think it's urgent. |
There was a problem hiding this comment.
I finally got this working for indexed search!
It's a bit weird in that I think it'll also work for non-artist entities in indexed search but only for artists everywhere else. Not sure if that's a concern; I suspect that getting primary aliases when loading other entity types from the DB will take a fair amount of additional code.
| $c->model('ArtistType')->load($artist, map { $_->target } @{ $artist->relationships_by_type('artist') }); | ||
| $c->model('Gender')->load($artist); | ||
| $c->model('Area')->load($artist); | ||
| my $lang = $c->stash->{current_language} // 'en'; |
There was a problem hiding this comment.
I'm pretty clueless about how this c (context?) stash stuff works. I'm passing the language around since current_language didn't seem to be available in load_related_info and the indexed search code, but please let me know if there's an easier way to get at it there.
Thanks! I've added "Primary alias in your language" title text. |
| $best_alias = $alias; | ||
| last; | ||
| } | ||
| # Otherwise, favor more-generic aliases (e.g. "en" over "en_US"). |
There was a problem hiding this comment.
_with_primary_alias in lib/MusicBrainz/Server/WebService/JSONSerializer.pm appears to also fall back to en and en_.*. This seems like it's probably desirable behavior for e.g. a Spanish user viewing a Japanese artist who only has an English alias, but it seems incorrect in many other cases.
This is something that's always bugged me a bit about aliases: there's no way to mark an alias as primary for all Latin-script languages, and there's also no locale associated with an entity's name, only with its aliases.
Let me know if there's something better I could be doing here. I think that what I'd really like would be to fall back to an alias using the same script as the user's language if the entity name isn't already using that script, but I'm not sure if that's possible.
There was a problem hiding this comment.
I think that what I'd really like would be to fall back to an alias using the same script as the user's language
I'm not sure if we have any mapping between language (or territory) and likely script (is this available in the CLDR?), but perhaps @yvanzo has an idea.
I do wonder if _with_primary_alias could be modified to make use of the proposed utility function, so that it can benefit from any future improvements in this area too.
There was a problem hiding this comment.
If it's okay with you, I'd prefer to avoid changing _with_primary_alias in this PR to reduce the (already high-seeming to me) odds that I break existing functionality. :-) Is adding a TODO okay for now?
|
I added some simple Selenium tests, although I assume they'll need some work (still no way to run them locally with musicbrainz-docker, right?). |
|
The new tests pass: |
|
@brainzbot, retest this please |
|
@reosarevok and @yvanzo, I think this is ready for review now (when you have time). :-) |
|
@mwiencek, would you mind taking a look at this one too? Thanks! @brainzbot, retest this please |
| $best_alias = $alias; | ||
| last; | ||
| } | ||
| # Otherwise, favor more-generic aliases (e.g. "en" over "en_US"). |
There was a problem hiding this comment.
I think that what I'd really like would be to fall back to an alias using the same script as the user's language
I'm not sure if we have any mapping between language (or territory) and likely script (is this available in the CLDR?), but perhaps @yvanzo has an idea.
I do wonder if _with_primary_alias could be modified to make use of the proposed utility function, so that it can benefit from any future improvements in this area too.
Try to find artists' primary aliases in the user's locale and display them alongside disambiguation comments in search results and artist pages.
| $best_alias = $alias; | ||
| last; | ||
| } | ||
| # Otherwise, favor more-generic aliases (e.g. "en" over "en_US"). |
There was a problem hiding this comment.
If it's okay with you, I'd prefer to avoid changing _with_primary_alias in this PR to reduce the (already high-seeming to me) odds that I break existing functionality. :-) Is adding a TODO okay for now?
Save aliases to MusicBrainz::Server::Data::Artist in load_related_info() so they only need to be loaded once and add a new find_best_primary_alias() utility function.
| @@ -286,26 +286,22 @@ sub show : PathPart('') Chained('load') | |||
| if (defined $base_name) { | |||
| $c->model('Relationship')->load_subset(['artist'], $base_name); | |||
| $c->stash( legal_name => $base_name ); | |||
| my $aliases = $c->model('Artist')->alias->find_by_entity_id($base_name->id); | |||
There was a problem hiding this comment.
Is it safe to assume that load_related_info will have saved the aliases to the Artist object whenever this is called, or should I include fallback code to try to load aliases if there aren't any?
It seems to work to the extent that I still see legal names when I view artist pages, at least. (Those weren't being shown with this change until I added the $c->model('Artist')->alias_type->load(@aliases) call in load_related_info.)
There was a problem hiding this comment.
Is it safe to assume that load_related_info will have saved the aliases to the Artist object whenever this is called, or should I include fallback code to try to load aliases if there aren't any?
It looks safe to me since load_related_info is called in after 'load', while show uses Chained('load').
There was a problem hiding this comment.
Maybe this isn't the case. https://beta.musicbrainz.org/artist/869cf6cc-07bb-470a-aa30-7e7e6000ffcf currently fails to load:
Can't use an undefined value as an ARRAY reference at lib/MusicBrainz/Server/Controller/Artist.pm line 295. at lib/MusicBrainz/Server/Controller/Artist.pm line 295
Catalyst::dispatch(?) called at lib/MusicBrainz/Server.pm line 400
There was a problem hiding this comment.
It works locally running the beta code, which suggests to me that the product of a prod load without aliases is being cached and causing the beta crash. If that's the case, we could either amend prod to save the info to the artist even if it doesn't use it, or amend beta temporarily to just show nothing rather than ISE if the data is missing. My preference would be the first, if we think it's safe (we're supposed to hotfix prod soon anyway for unrelated reasons)
There was a problem hiding this comment.
@mwiencek and @reosarevok, just to make sure I understand this properly, will the caching also cause issues with the primary_alias attribute on Entity::Artist? The alias is selected based on the UI language. Is that language incorporated into the cache keys, or will there be issues if an entity is cached for a request from a French-using user and then later reused for a request from an English-using user?
There was a problem hiding this comment.
That shouldn't be an issue as entities are cached immediately before any additional data is loaded onto them. So no primary_alias value should be set on the cached copies.
| @@ -740,6 +745,19 @@ sub schema_fixup | |||
| $data->{'artist_credit'} = MusicBrainz::Server::Entity::ArtistCredit->new( { names => \@credits } ); | |||
| } | |||
|
|
|||
| if (defined $data->{aliases}) { | |||
| my @aliases = map { | |||
| MusicBrainz::Server::Entity::Alias->new( | |||
There was a problem hiding this comment.
This seems to work, but please let me know if I should be doing something else to create these objects.
| (!defined $best || length($alias->locale) == 2)) { | ||
| $best = $alias; | ||
| } | ||
| # If we find an English alias, use it as a fallback. This is likely |
There was a problem hiding this comment.
I figured it'd be worthwhile falling back to English here since it looks like all of the UI languages that are presently listed use the Latin script.
There was a problem hiding this comment.
I think I mentioned it earlier, but a (future?) downside of this is that if e.g. a user selects Japanese as their UI language (I don't think they can at present), then they'll always see English aliases for Japanese artists, since I don't think there's any way to know that the artist entity's name is already in Japanese.
There was a problem hiding this comment.
You could in theory check if there's a Japanese artist alias that exactly matches the artist name (but you're right, if that data doesn't exist).
There was a problem hiding this comment.
Yeah, in practice I don't think that many editors add primary aliases that are identical to entity names. https://musicbrainz.org/artist/9ddd7abc-9e1b-471d-8031-583bc6bc8be9/aliases has a zillion aliases, but of the two matching the "Пётр Ильич Чайковский" entity name, one is a non-primary legal name for Russian and the other is a search hint. Oddly, there's a primary artist name Russian alias of "Пётр Чайковский".
|
@mwiencek, mind taking another look? |
Fetch aliases for all artists at once, use Moose setters, and clear aliases from the search API so Entity::Artist's aliases attribute can use the Entity::Alias type.
Save Entity::Alias objects in schema_fixup and update title text to "Primary alias".
|
Thanks for all the help getting it merged! |
* master: Update POT files using the production database MBS-7646: Show localized artist aliases in more places (#3191) MBS-13558: Convert /work templates to use Flow component syntax MBS-13558: Convert /voting templates to use Flow component syntax MBS-13558: Convert /user templates to use Flow component syntax MBS-13558: Convert /url templates to use Flow component syntax MBS-13558: Convert /taglookup templates to use Flow component syntax MBS-13558: Convert /tag templates to use Flow component syntax MBS-13558: Convert /statistics templates to use Flow component syntax MBS-13558: Convert /series templates to use Flow component syntax Remove unnecessary explicitness while passing index MBS-13558: Convert /search templates to use Flow component syntax MBS-13558: Convert /release_group templates to use Flow component syntax Move type DeleteReleaseFormT to DeleteRelease MBS-13558: Convert /release templates to use Flow component syntax MBS-13558: Convert /relationship templates to use Flow component syntax Move type DeleteRecordingFormT to DeleteRecording MBS-13558: Convert /recording templates to use Flow component syntax MBS-13558: Convert /place templates to use Flow component syntax MBS-13558: Convert /otherlookup templates to use Flow component syntax MBS-13558: Convert /oauth2 templates to use Flow component syntax MBS-13558: Convert /medium templates to use Flow component syntax MBS-13558: Convert /mbid templates to use Flow component syntax MBS-13558: Convert /main templates to use Flow component syntax MBS-13558: Convert /label templates to use Flow component syntax MBS-13558: Convert /isrc, /iswc templates to use Flow component syntax Move type DeleteInstrumentFormT to DeleteInstrument MBS-13558: Convert /instrument templates to use Flow component syntax Move type DeleteGenreFormT to DeleteGenre MBS-13558: Convert /genre templates to use Flow component syntax MBS-13558: Convert /event templates to use Flow component syntax Move type DeleteAliasFormT to DeleteAlias MBS-13558: Convert /entity templates to use Flow component syntax Fix typo MBS-13558: Convert /elections templates to use Flow component syntax MBS-13558: Convert /edit_note templates to use Flow component syntax MBS-13558: Convert /doc templates to use Flow component syntax MBS-13558: Convert /collection templates to use Flow component syntax MBS-13558: Convert /cdtoc templates to use Flow component syntax MBS-13558: Convert /cdstub templates to use Flow component syntax MBS-13558: Convert /artist_credit templates to use Flow component syntax MBS-13558: Convert /artist templates to use Flow component syntax Move type AreaDeleteFormT to DeleteArea MBS-13558: Convert /area templates to use Flow component syntax MBS-13558: Convert /annotation templates to use Flow component syntax Move type WikiDocT to WikiDocIndex MBS-13558: Convert /admin templates to use Flow component syntax MBS-13558: Convert /account templates to use Flow component syntax Disable eslint rules that conflict with Flow component syntax Update flow to 0.234.0 (#3242) MBS-13548: obey returnto for already logged in users (#3241) Revert "Bind cpanfile and snapshot to install Perl modules" Install git to list translation files
* beta: Translated using Weblate (French) Translated using Weblate (Spanish) Translated using Weblate (Italian) Translated using Weblate (Greek) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Norwegian Bokmål) Translated using Weblate (Spanish) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Russian) Translated using Weblate (Spanish) Translated using Weblate (Lithuanian) Update translation files Translated using Weblate (Korean) Update POT files using the production database Fix `Controller::User::Collections` test MBS-13563: Italicize aliases and sort names in Autocomplete2 (#3248) Translated using Weblate (Korean) Translated using Weblate (Russian) Translated using Weblate (Spanish) Translated using Weblate (Lithuanian) Update translation files Add a test for MBS-13570 Fix extraneous dereference MBS-13570: Avoid crash in artist collection load_related_info Update POT files using the production database Translated using Weblate (Spanish) Translated using Weblate (Chinese (Traditional)) Translated using Weblate (Lithuanian) Translated using Weblate (Italian) Update translation files MBS-13533: Fix order of cover art in release group artwork (#3238) Update POT files using the production database MBS-7646: Show localized artist aliases in more places (#3191) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Italian) Translated using Weblate (Swedish) Translated using Weblate (Spanish (Latin America)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Turkish) Translated using Weblate (Turkish) Translated using Weblate (Spanish) Translated using Weblate (Persian) Translated using Weblate (Russian) Translated using Weblate (Portuguese) Translated using Weblate (Norwegian Bokmål) Translated using Weblate (Korean) Translated using Weblate (Croatian) Translated using Weblate (Catalan) Translated using Weblate (Lithuanian) Translated using Weblate (Lithuanian) Translated using Weblate (Japanese) Translated using Weblate (Japanese) Translated using Weblate (Romanian) Translated using Weblate (Dutch) Translated using Weblate (Italian) Translated using Weblate (French) Translated using Weblate (Finnish) Translated using Weblate (Estonian) Translated using Weblate (French) Translated using Weblate (French) Translated using Weblate (Spanish) Translated using Weblate (German) Translated using Weblate (German) Translated using Weblate (German) Translated using Weblate (German) Translated using Weblate (German) Translated using Weblate (Greek) Translated using Weblate (Lithuanian) Translated using Weblate (Lithuanian) MBS-13558: Convert /work templates to use Flow component syntax MBS-13558: Convert /voting templates to use Flow component syntax MBS-13558: Convert /user templates to use Flow component syntax MBS-13558: Convert /url templates to use Flow component syntax MBS-13558: Convert /taglookup templates to use Flow component syntax MBS-13558: Convert /tag templates to use Flow component syntax MBS-13558: Convert /statistics templates to use Flow component syntax MBS-13558: Convert /series templates to use Flow component syntax Remove unnecessary explicitness while passing index MBS-13558: Convert /search templates to use Flow component syntax MBS-13558: Convert /release_group templates to use Flow component syntax Move type DeleteReleaseFormT to DeleteRelease MBS-13558: Convert /release templates to use Flow component syntax MBS-13558: Convert /relationship templates to use Flow component syntax Move type DeleteRecordingFormT to DeleteRecording MBS-13558: Convert /recording templates to use Flow component syntax MBS-13558: Convert /place templates to use Flow component syntax MBS-13558: Convert /otherlookup templates to use Flow component syntax MBS-13558: Convert /oauth2 templates to use Flow component syntax MBS-13558: Convert /medium templates to use Flow component syntax MBS-13558: Convert /mbid templates to use Flow component syntax MBS-13558: Convert /main templates to use Flow component syntax MBS-13558: Convert /label templates to use Flow component syntax MBS-13558: Convert /isrc, /iswc templates to use Flow component syntax Move type DeleteInstrumentFormT to DeleteInstrument MBS-13558: Convert /instrument templates to use Flow component syntax Move type DeleteGenreFormT to DeleteGenre MBS-13558: Convert /genre templates to use Flow component syntax MBS-13558: Convert /event templates to use Flow component syntax Move type DeleteAliasFormT to DeleteAlias MBS-13558: Convert /entity templates to use Flow component syntax Fix typo MBS-13558: Convert /elections templates to use Flow component syntax MBS-13558: Convert /edit_note templates to use Flow component syntax MBS-13558: Convert /doc templates to use Flow component syntax MBS-13558: Convert /collection templates to use Flow component syntax MBS-13558: Convert /cdtoc templates to use Flow component syntax MBS-13558: Convert /cdstub templates to use Flow component syntax MBS-13558: Convert /artist_credit templates to use Flow component syntax MBS-13558: Convert /artist templates to use Flow component syntax Move type AreaDeleteFormT to DeleteArea MBS-13558: Convert /area templates to use Flow component syntax MBS-13558: Convert /annotation templates to use Flow component syntax Move type WikiDocT to WikiDocIndex MBS-13558: Convert /admin templates to use Flow component syntax MBS-13558: Convert /account templates to use Flow component syntax Disable eslint rules that conflict with Flow component syntax Update flow to 0.234.0 (#3242) MBS-13548: obey returnto for already logged in users (#3241) Revert "Bind cpanfile and snapshot to install Perl modules" Install git to list translation files Revert "Bind cpanfile and snapshot to install Perl modules" Install git to list translation files Remove duplicate instrument in Lithuanian PO file Translated using Weblate (Lithuanian) Update translation files Update POT files using the production database Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Italian) Translated using Weblate (Russian) Update translation files Translated using Weblate (Lithuanian) Bump musicbrainz-tests image tag after MBS-13358 Regenerate cpanfile.snapshot files after MBS-13358 Update Perl dependencies for internationalization MBS-13546: Switch to Email::Address::XS Update DateTime::Locale version to 1.41 MBS-13358 (II.2): Require Perl 5.38.0 or later Bind install_svlogd_services.sh instead of copying Replace RUN chmod/chown with COPY --chmod/--chown Always set PERL_CARTON_PATH in Docker images Bind cpanfile and snapshot to install Perl modules Always set ownership for cpanm/Carton directories Always install Perl and cpanm/Carton in one RUN Cache cpanm downloads, builds, and tests Reinstall perl modules only when cpanfile changes Drop instruction included in install_perl_modules Always clean up the cpanm cache for root user MBS-13358 (II.1): Upgrade Perl version to 5.38.2 Install ts from source instead of using apt-get Install carton from cpanm instead of using apt-get Drop support for Perl 5.30 Update Flow to v0.233.0 (#3237) MBS-13540: Update track credits in a few more cases (#3233) MBS-13541: Accept uppercase MBIDs in JavaScript (#3235) Install xz through APT as it isn’t so new anymore Install APT packages using self-documented macros Factorize setting up cpanm & carton through macros Templatize Dockerfile.tests to reuse macros Rework installing cpanm after official docker-perl Document Perl deps installation through comments Replace the few `wget` calls with `curl` calls Use build ARG instruction for installed XZ version Remove temporary keys to install XZ utils Add temporary keyrings through bind mount Format lists of packages one by line Generalize options passed to curl Factorize copy_common_mbs_files into server_base Drop duplicate ARG already provided in server_base Cache APT directories when building Docker images MBS-13539: Update VK logo (#3232) Add context to "order" header (#3229) Update POT files using the production database Benchmark createFastObjectClone with Node.js v20 Amend commit 954dae4: Upgrade Node.js to v20
MBS-7646
Try to find artists' primary aliases in the user's locale and display them alongside disambiguation comments in search results and artist pages.
Problem
Editors argue about whether Latin-script versions of artist names should be included in disambiguations (see e.g. https://musicbrainz.org/edit/109219215). Even when this is done, it's not a great approach since it doesn't help editors who are using UI languages based on other scripts. Without being able to see localized names in search results, editors either look at sort names (if they know about them) or end up creating duplicate entities.
Solution
Localized versions of artist names are already stored as aliases, so automatically display those in italicized text at the beginning of the disambiguation comment when possible, e.g.
I'm only including the alias when it doesn't already match the artist name, and I'm performing fallback to favor an exact language match before falling back to the generic language and then other variants (e.g.
en_UKfavorsen_UK, thenen, and thenen_US). I'm not sure if the fallback is important given the current UI language options, though.Testing
I did manual testing by doing indexed and direct searches for
tchaikovskywith various UI languages and checking that the appropriate alias is shown, and also checking that aliases are displayed in the header on artist pages.I also added a new Selenium test.